-
Notifications
You must be signed in to change notification settings - Fork 30
simplify integration tests #148
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
simplify integration tests #148
Conversation
e2a0e29
to
465ded9
Compare
465ded9
to
53c4a2e
Compare
faf1721
to
299ee90
Compare
@dicej there is an issue with the latest nightly. Is nightly still necessary for the runtime? In which case I will maybe either allow the lint or opt into the feature it is flagging |
Wait nevermind I just realized I didnt try the latest nightly locally. I'll do that later |
Ok it appears my wait on stderr doesn't work on windows... and this hangs forever... sorry... |
baef216
to
70f033b
Compare
70f033b
to
f7dc207
Compare
@dicej This is ready (I think) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this!
LGTM overall; just wondering if we could drop the sleep
in http_example
and boost the number of retries, as well as switch to an exponential back-off (e.g. starting at 100ms and capped at 20 seconds or so).
tests/componentize.rs
Outdated
.spawn()?; | ||
|
||
// Sleep a bit to give the server time to start | ||
std::thread::sleep(Duration::from_secs(5)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary given the retry
code below? I'd rather do some kind of retry with exponential back-off (e.g. no initial delay, then 100ms, then 200, then 400, etc.) to ensure that the tests run quickly on fast machines but also run reliably on slower machines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I got a little sidetracked just trying to get this to work 😅
I pushed up the change, let me know what you think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Finally getting around to replacing the need for all of these commands, and instead just uses rust crates/builtins.
I haven't replaced the wasmtime calls yet.